Skip to content

Refactor pagination to prepare for cursor-based pagination. #1807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 5, 2019

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Aug 18, 2019

This continues the changes made in #1763. Since that PR was merged, one
of the non-code steps has been taken care of -- All users hitting any
endpoint with ?page=20 (which is an arbitrary search pattern that
seemed high enough to give any crawlers going through pagination) have
been contacted about the change, with a PR opened against any that
included a repo. (Intersting aside, there are zero records of this for
any endpoint other than search, which perhaps implies we can get rid of
a few of these endpoints, but that's a separate discussion).

This PR does not change any functionality, but moves some code around
to better encapsulate things for upcoming changes. Specifically:

  • Change our frontend to show "next/prev page" links on the all crates
    page
  • Stop returning the "total" meta item when the next/prev page links
    will be cursor based (which I'd actually just like to start omitting
    in general)

The main goal of this change was to stop having any code outside of
Paginated (which has been renamed to PaginatedQuery, as there's a
new type called Paginated) care about how pagination occurs. This
means other code can't care about how pagination happens (with the
exception of reverse_dependencies, which uses raw SQL, and sorta has
to... That was a bit of a rabbit hole, see
diesel-rs/diesel#2150 for details. Given the
low traffic to that endpoint, I think we can safely ignore it).

The distribution of responsibilities is as follows:

  • PaginatedQuery<T>: Given the query params, decides how to paginate
    things, generates appropriate SQL, loads a Paginated<T>.
  • Paginated<T>: Handles providing an iterator to the records, getting
    the total count (to be removed in the near future), providing the
    next/prev page params
  • Request: Takes the pagination related query params, turns that into
    an actual URL (note: Due to jankiness in our router, this will only
    ever be a query string, we have no way of getting the actual path)

The next step from here is to change our UI to stop showing page
numbers, and then remove the total field.

This PR will introduce a performance regression that was addressed by
#1668. That PR was addressing "this will become a problem in a future",
not "this is about to take the site down". Given the intent to remove
the total field entirely, I think it is fine to cause this regression
in the short term. If we aren't confident that the changes to remove
this field will land in the near future, or want to be conservative
about this, I can add some additional complexity/git churn to retain the
previous performance characteristics

This continues the changes made in rust-lang#1763. Since that PR was merged, one
of the non-code steps has been taken care of -- All users hitting any
endpoint with `?page=20` (which is an arbitrary search pattern that
seemed high enough to give any crawlers going through pagination) have
been contacted about the change, with a PR opened against any that
included a repo. (Intersting aside, there are *zero* records of this for
any endpoint other than search, which perhaps implies we can get rid of
a few of these endpoints, but that's a separate discussion).

This PR does not change any functionality, but moves some code around
to better encapsulate things for upcoming changes. Specifically:

- Change our frontend to show "next/prev page" links on the all crates
  page
- Stop returning the "total" meta item when the next/prev page links
  will be cursor based (which I'd actually just like to start omitting
  in general)

The main goal of this change was to stop having any code outside of
`Paginated` (which has been renamed to `PaginatedQuery`, as there's a
new type called `Paginated`) care about how pagination occurs. This
means other code can't care about *how* pagination happens (with the
exception of `reverse_dependencies`, which uses raw SQL, and sorta has
to... That was a bit of a rabbit hole, see
diesel-rs/diesel#2150 for details. Given the
low traffic to that endpoint, I think we can safely ignore it).

The distribution of responsibilities is as follows:

- `PaginatedQuery<T>`: Given the query params, decides how to paginate
  things, generates appropriate SQL, loads a `Paginated<T>`.
- `Paginated<T>`: Handles providing an iterator to the records, getting
  the total count (to be removed in the near future), providing the
  next/prev page params
- `Request`: Takes the pagination related query params, turns that into
  an actual URL (note: Due to jankiness in our router, this will only
  ever be a query string, we have no way of getting the actual path)

The next step from here is to change our UI to stop showing page
numbers, and then remove the `total` field.

This PR will introduce a performance regression that was addressed by
 rust-lang#1668. That PR was addressing "this will become a problem in a future",
not "this is about to take the site down". Given the intent to remove
the `total` field entirely, I think it is fine to cause this regression
in the short term. If we aren't confident that the changes to remove
this field will land in the near future, or want to be conservative
about this, I can add some additional complexity/git churn to retain the
previous performance characteristics
@rust-highfive
Copy link

r? @jtgeibel

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys
Copy link
Contributor

kzys commented Aug 27, 2019

It seems clippy is still not happy.

https://travis-ci.com/rust-lang/crates.io/jobs/226409422#L499

I swear I run this locally...
Copy link
Member

@jtgeibel jtgeibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks great to me, and I saw no issues when testing locally.

Feel free to include this in the next deploy if you want. Otherwise I'll r+ soon.

@sgrif
Copy link
Contributor Author

sgrif commented Sep 5, 2019

@bors r=jtgeibel

@bors
Copy link
Contributor

bors commented Sep 5, 2019

📌 Commit 8ef5179 has been approved by jtgeibel

@bors
Copy link
Contributor

bors commented Sep 5, 2019

⌛ Testing commit 8ef5179 with merge 6514cdb...

bors added a commit that referenced this pull request Sep 5, 2019
Refactor pagination to prepare for cursor-based pagination.

This continues the changes made in #1763. Since that PR was merged, one
of the non-code steps has been taken care of -- All users hitting any
endpoint with `?page=20` (which is an arbitrary search pattern that
seemed high enough to give any crawlers going through pagination) have
been contacted about the change, with a PR opened against any that
included a repo. (Intersting aside, there are *zero* records of this for
any endpoint other than search, which perhaps implies we can get rid of
a few of these endpoints, but that's a separate discussion).

This PR does not change any functionality, but moves some code around
to better encapsulate things for upcoming changes. Specifically:

- Change our frontend to show "next/prev page" links on the all crates
  page
- Stop returning the "total" meta item when the next/prev page links
  will be cursor based (which I'd actually just like to start omitting
  in general)

The main goal of this change was to stop having any code outside of
`Paginated` (which has been renamed to `PaginatedQuery`, as there's a
new type called `Paginated`) care about how pagination occurs. This
means other code can't care about *how* pagination happens (with the
exception of `reverse_dependencies`, which uses raw SQL, and sorta has
to... That was a bit of a rabbit hole, see
diesel-rs/diesel#2150 for details. Given the
low traffic to that endpoint, I think we can safely ignore it).

The distribution of responsibilities is as follows:

- `PaginatedQuery<T>`: Given the query params, decides how to paginate
  things, generates appropriate SQL, loads a `Paginated<T>`.
- `Paginated<T>`: Handles providing an iterator to the records, getting
  the total count (to be removed in the near future), providing the
  next/prev page params
- `Request`: Takes the pagination related query params, turns that into
  an actual URL (note: Due to jankiness in our router, this will only
  ever be a query string, we have no way of getting the actual path)

The next step from here is to change our UI to stop showing page
numbers, and then remove the `total` field.

This PR will introduce a performance regression that was addressed by
 #1668. That PR was addressing "this will become a problem in a future",
not "this is about to take the site down". Given the intent to remove
the `total` field entirely, I think it is fine to cause this regression
in the short term. If we aren't confident that the changes to remove
this field will land in the near future, or want to be conservative
about this, I can add some additional complexity/git churn to retain the
previous performance characteristics
@bors
Copy link
Contributor

bors commented Sep 5, 2019

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing 6514cdb to master...

@bors bors merged commit 8ef5179 into rust-lang:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants